release/21.x: [LLDB][ProcessWindows] Set exit status on instance rather than going through all targets (#159308) #161541
Conversation
…through all targets (llvm#159308) When quitting LLDB on Windows while a process was still running, LLDB would take unusually long to exit. This was due to a temporary deadlock: The main thread was destroying the processes. In doing so, it iterated over the target list: https://github.com/llvm/llvm-project/blob/88c64f76ed2ca226da99b99f60d316b1519fc7d8/lldb/source/Core/Debugger.cpp#L1095-L1098 This locks the list for the whole iteration. Finalizing the process would eventually lead to `DebuggerThread::StopDebugging`, which terminates the process and waits for it to exit: https://github.com/llvm/llvm-project/blob/88c64f76ed2ca226da99b99f60d316b1519fc7d8/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp#L196 The debugger loop (on a separate thread) would see that the process exited and call [`ProcessWindows::OnExitProcess`](https://github.com/llvm/llvm-project/blob/88c64f76ed2ca226da99b99f60d316b1519fc7d8/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp#L656-L673). This calls the static function [`Process::SetProcessExitStatus`](https://github.com/llvm/llvm-project/blob/0a7a7d56fc882653335beba0d1f8ea9f26089c22/lldb/source/Target/Process.cpp#L1098-L1126). This tries to find the process by its ID from the debugger's target list. Doing so requires locking the list, so the debugger thread would then be stuck on https://github.com/llvm/llvm-project/blob/0a7a7d56fc882653335beba0d1f8ea9f26089c22/lldb/source/Target/TargetList.cpp#L403 After 5s, the main thread would give up waiting. So every exit where the process was still running would be delayed by about 5s. Since `ProcessWindows` would find itself when calling `SetProcessExitStatus`, we can call `SetExitStatus` directly. This can also make some tests run faster. For example, the DIA PDB tests previously took 15s to run on my PC (24 jobs) and now take 5s. For all shell tests, the difference isn't that big (only about 3s), because most don't run into this and the tests run in parallel. (cherry picked from commit a868f28)
|
@DavidSpickett What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-lldb Author: None (llvmbot) ChangesBackport a868f28 Requested by: @Nerixyz Full diff: https://github.com/llvm/llvm-project/pull/161541.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 27530f032ce51..0fecefe23b88e 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -666,7 +666,7 @@ void ProcessWindows::OnExitProcess(uint32_t exit_code) {
target->ModulesDidUnload(unloaded_modules, true);
}
- SetProcessExitStatus(GetID(), true, 0, exit_code);
+ SetExitStatus(exit_code, /*exit_string=*/"");
SetPrivateState(eStateExited);
ProcessDebugger::OnExitProcess(exit_code);
|
So I am in favour of backporting this. Assuming release managers are willing to accept performance changes like this one. |
There was a problem hiding this comment.
It seems like this wasn't merged for 21.1.3, even though this did have essentially an approval from @DavidSpickett (plus a thumbs-up from @JDevlieghere), even though it didn't have the formal "approved" status. So if there's no other feedback here, I'll approve it, and if nobody else minds, let's merge it for 21.1.4.
github-project-automation
bot
moved this from Needs Review
to Needs Merge
in LLVM Release Status
|
Well I think we needed a release manager to agree with us agreeing to merge it, perhaps it wasn't in the right state for them to see it. I believe @c-rhodes is handling 21.1.4, they have the final say here I think. |
Which might have been because I didn't actually click the buttons to approve, thinking that the release manager would do that, but they probably wanted to see me approve it first. Sorry about that, lesson learned. |
|
seems reasonable to me, I'll make sure this gets in for 21.1.4. |
Backport a868f28
Requested by: @Nerixyz